Add into_text_value() to PrimitiveValues#718
Conversation
techtenk
changed the title
techtenk
force-pushed
the
techtenk/into_text_value
branch
from
d0e4053 to
f3af414
Compare
Enet4
left a comment
Enet4
left a comment
There was a problem hiding this comment.
Thank you for your pull request. Please let me know if you are available to reiterate on it per the comments below.
The only major concern that makes me put this on hold is the fact that it can easily create invalid date-time values.
| /// | ||
| /// assert_eq!( | ||
| /// text_value, | ||
| /// PrimitiveValue::Str("2024-12-25".to_string()) |
There was a problem hiding this comment.
This shows... an interesting caveat. Dates are only DICOM-encoded correctly when converted via to_encoded, but to_raw_str does not use it (which would be fine because to_str and to_raw_str are ill-advised for DICOM serialization anyway, so they are not expected to be pushed back into a DICOM value).
The current behavior of this method will come across as surprising, so we will have to adjust it accordingly.
- Date and date-time values in
PrimitiveValue::DateandPrimitiveValue::DateTimeare encoded to their standard DICOM textual form.- Other binary variants are converted to strings via
to_string.
There was a problem hiding this comment.
I can work on this. For clarification, is it as simple as calling to_encoded() when appropriate and updating the docs? Or am I missing some nuance?
There was a problem hiding this comment.
So yes, for the variants Da and Dt they should be converted to text via to_encoded(). The rule for concatenating multiple values into a single string would be the same (joined together with backslash as the separator).
Co-authored-by: Eduardo Pinho <enet4mikeenet@gmail.com>
Co-authored-by: Eduardo Pinho <enet4mikeenet@gmail.com>
| .unwrap()]); | ||
| assert_eq!( | ||
| value.into_text_value(), | ||
| PrimitiveValue::Str("2012-12-21 09:30:01".to_string()) |
There was a problem hiding this comment.
As discussed in past comments, dates and times (DA, TM, DT) should continue to comply with their value representation after being converted to text, as per PS3.5 section 6.2. This test, the implementation, and the documentation of the method need to be updated accordingly.
| PrimitiveValue::Str("2012-12-21 09:30:01".to_string()) | |
| PrimitiveValue::Str("20121221093001".to_string()) |
2 participants
Implements: ISSUE-88
This adds
into_text_value()forPrimitiveValueas well as animpl From<Cow<'_, str>> for PrimitiveValueper the suggestion in the issue above.Assumptions to check:
PrimitiveValue::Str(includingEmptyandStrs)to_raw_str()instead ofto_str()to preserve the most information (no trimming of extra spaces, etc)into_text_value()andimpl From<Cow<'_, str>> for PrimitiveValueare useful for convenience/completeness, even thoughinto_text_value()is justPrimitiveValue::from(self.to_raw_str())Other things to check:
to_raw_str()tests. Let me know if I should.